Humble apologies for taking so long to getting to this PR @lpoug. I've unfortunately not had much time to spend on CausalPy as I'd have liked, but hoping to catch up with the backlog.

There are currently a couple of issues with the remote checks. I'm hoping to get these resolved in #437, at which point I'll test this out locally and give feedback if necessary before we can merge this :)

Codecov Report

Attention: Patch coverage is 96.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 94.53%. Comparing base (2a6f9db) to head (da6c91d).
Report is 39 commits behind head on main.

@@            Coverage Diff             @@
##             main     #438      +/-   ##
==========================================
+ Coverage   94.40%   94.53%   +0.12%     
==========================================
  Files          31       31              
  Lines        1985     2068      +83     
==========================================
+ Hits         1874     1955      +81     
- Misses        111      113       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Humble apologies for taking so long to getting to this PR @lpoug. I've unfortunately not had much time to spend on CausalPy as I'd have liked, but hoping to catch up with the backlog.

There are currently a couple of issues with the remote checks. I'm hoping to get these resolved in #437, at which point I'll test this out locally and give feedback if necessary before we can merge this :)

Absolutely no problem whatsoever @drbenvincent! Let me know when the time comes, I'll be around 😄

Hi @lpoug. I pushed some changes, can you make sure to pull the latest version?

I'll try to review this in the next few days :)

Hey there @drbenvincent. Just to be sure, are you waiting on anything on my side?

Apologies for the delay! Just dropping in some review comments now

Sorry about the slow review on this. My bad.

Overall this looks good. I've suggested some minor changes. Other than that the main thing is to update the tests to ensure this functionality remains working into the future.

Could you add new tests to test_integration_pymc_examples.py and test_integration_skl_examples.py. I imagine we can just test that we successfully get back a dataframe from calling result.get_plot_data on the experiments that you've implemented so far. You could optionally test that the contents of that dataframe is as expected, e.g. has the desired columns.

In theory an ultra pedantic person might want to test that we get an exception when calling the get_plot_data on experiments that.

Because this PR involves additional methods, can you run make uml. This should update the UML diagram that we include in CONTRIBUTING.md

Sorry again about the latency on this review.

No problem at all! I was just starting to get worried about something that still needed to be done on my end 😅

Thank you for the reviews. I've added links to the commits directly in your comments. 6a6face (renaming of functions)

Regarding tests, I have added them in 97f0d79

I have not added anything yet to test that we get an exception when calling the get_plot_data on experiments for which it is not implemented. I'll try to take a moment to think about how to do so precisely.

Finally, I have updated the diagrams in 0edca77

Let me know if these changes look good or not or if you had anything else in mind!

I made the changes for dynamic naming of *_hdi_upper and *_hdi_lower and adjusted the tests accordingly (please see my comment above regarding tests). I also updated the docstrings of the tests as requested.
See commit 44d3870

Regarding the merge conflicts, to be honest I'm not sure what I need to do on my side? Could you enlighten me, please?

Thanks!

Hi @lpoug. I've sorted the conflicting files and done a few small things. I'm just noticing that at the moment the rendered docs obscure the allowable args/kwargs. So we have this:

Same situation if we look at an actual experiment class:

I will have a quick play with this to see if we can expose the parameter information to the user in the docs. I think the easiest way is to revert a previous suggestion and make _get_plot_data_bayesian and _get_plot_data_ols public methods again. I experiment and make a commit shortly.

So now the docs expose the kwargs to the user. It's not ultra clean because we wanted to hide the bayesian vs ols functions, but at least this way the user can find out what (if any) kwargs they can pass. Here are some examples:

Those functions are clickable, which takes you to this, for example

And the arviz.hdi is also clickable and takes you through to the external docs.

Any other final changes @lpoug, or are you happy to merge this now?

Looks like there might just be a quick test coverage issue to deal with, but I'm sure we can get that done between us.

Success with code coverage and all tests passing. Hope you don't mind that I carried this over the finish like @lpoug. Sorry the process was a little slow, it will be faster on your next PR 😀

Amazing thank you @drbenvincent! Glad to have been a part of this 😄 Looking forward for the next, even more challenging one!